Skip to content

fix(security): allow TLS terminator Origin scheme skew for management - #779

Merged
Ingwannu merged 4 commits into
devfrom
fix/760-management-origin-tls
Aug 1, 2026
Merged

fix(security): allow TLS terminator Origin scheme skew for management#779
Ingwannu merged 4 commits into
devfrom
fix/760-management-origin-tls

Conversation

@Wibias

@Wibias Wibias commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fixes #760

Allows TLS terminator Origin scheme skew for management API requests.

Security

Credential-aware / corsAllowOrigins; session origin+CSRF unchanged.

Test plan

  • CI green
  • Management origin/TLS regression tests

@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@Wibias, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 18 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 203813eb-6d64-4615-b405-f8d5fa33474e

📥 Commits

Reviewing files that changed from the base of the PR and between 33caf33 and 208cbe0.

📒 Files selected for processing (3)
  • src/server/auth-cors.ts
  • tests/management-origin-tls.test.ts
  • tests/storage-cleanup.test.ts

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ca58f0bcf9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/server/auth-cors.ts Outdated
Comment thread src/server/auth-cors.ts Outdated
@github-actions github-actions Bot added the bug Something isn't working label Jul 30, 2026
@Wibias

Wibias commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator Author

[shipping-github] Merge-ready

@Wibias

Wibias commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator Author

[shipping-github] Security sanity (redacted) — quick pass on TLS-terminator Origin handling.

  • Scope: Management/CORS origin checks only; allows same-host http/https skew when a reverse proxy terminates TLS (Origin https vs process http), still fail-closed on port/scheme mismatches within the same scheme.
  • Boundary: Does not widen token validation or disable auth; extra origins still require explicit \corsAllowOrigins.
  • Tests: \management-origin-tls\ / auth-cors regression coverage present.
  • Residual risk: Misconfigured extra origins or hostile proxies that forge Origin remain the main abuse surface — unchanged from baseline.

No secrets or tokens logged in diff.

@Wibias

Wibias commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator Author

Verdict: gated

  • Usefulness: Fixes Valid admin-token /api/* requests are 403'd behind a TLS-terminating reverse proxy #760 for admin-token deployments behind TLS terminators (scheme skew + tightened port matching).
  • Bugs: session-cookie Origin equality still http-vs-https mismatched behind TLS — follow-up if session mode must work there.
  • Security: Origin relaxation is hostname-matched https→http only; management token auth still required. Maintainer security review required (auth/CORS boundary).
  • Owner/maintainer reviews: security sign-off needed.
  • Bots: addressed / declined docs follow-up.
  • Base / CI: PR tests green; windows suite noise unrelated.
  • Gate: security review + CI clarity.

@lidge-jun

Copy link
Copy Markdown
Owner

Reviewed alongside the other open security-boundary work. The defect is real and this fixes it, but I would like a narrower version before it lands.

The defect. /api/* first passes requireManagementAuth() (src/server/index.ts:391-395), then hits a separate origin gate (src/server/management-api.ts:82-85) that demands byte-for-byte equality with the process-observed http://Host origin (src/server/auth-cors.ts:91-110). Behind a TLS terminator the browser sends https://… and the proxy observes http://…, so a correctly authenticated request is refused. Confirmed on current dev.

Where the PR goes further than the report needs. It accepts a cross-scheme same-host relationship automatically, with no trusted-proxy setting. Raw callers and anything that can reach the backend directly can forge both Host and Origin — that does not bypass management auth, which runs first and compares the token in constant time (management-auth.ts:195-197), and GUI sessions still require exact origin equality plus CSRF (:198-215). So this is not an auth bypass. But it does make origin admission depend on an untrusted Host by default, for every deployment, to serve a configuration the operator already declares explicitly.

The narrower shape. The repository already has isExtraAllowedOrigin() for exactly this. Changing management admission to

!origin || origin === requestOrigin || isExtraAllowedOrigin(origin, config)

widens only to origins the operator listed in corsAllowOrigins — including the https://proxy.example.com the reporter had already configured — and makes preflight work through auth-cors.ts:134-140. Same outcome for the reported deployment, no default change for anyone else.

Negative test either way. With a remote bind and corsAllowOrigins: ["https://proxy.example.com"]: that HTTPS origin succeeds for both OPTIONS and an authenticated mutation, while https://evil.example.com and https://proxy.example.com:4443 stay 403. Session-origin and CSRF tests unchanged.

Not closing — the diagnosis is right and the fix is close. @lidge-jun @Ingwannu for the security review that MAINTAINERS.md requires here.

@Wibias

Wibias commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator Author

[shipping-github] Addressed owner feedback — narrowed management Origin admission to corsAllowOrigins only (removed automatic cross-scheme Host match).

@Wibias

Wibias commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator Author

Ready to merge @lidge-jun @Ingwannu

Wibias added 4 commits July 31, 2026 12:33
Only allow https-to-http terminator skew for matching public ports (or the 443/80 default pair) and reject unrelated cross-port origins.
Drop automatic cross-scheme Host matching so management origin widens only to origins the operator listed, matching the reported reverse-proxy setup without a default change for other deployments.
The permanent spawn/dynamic-tool cleanup assertion hit Bun's default 5s
timeout on windows-latest for PR #779; give it STORE_BUDGET_MS like the
other SQLite cleanup paths.
@Wibias
Wibias force-pushed the fix/760-management-origin-tls branch from d92da4e to 208cbe0 Compare July 31, 2026 10:35

@Ingwannu Ingwannu left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed current head 208cbe0 against current dev. The earlier automatic cross-scheme Host relaxation is gone: admission expands only to exact operator-declared corsAllowOrigins, while admin-token authentication, GUI session origin equality, and CSRF remain unchanged. Focused origin/auth tests (20) and typecheck pass locally. Maintainer auth-boundary sign-off: approved.

@Ingwannu
Ingwannu merged commit aa924f3 into dev Aug 1, 2026
15 of 16 checks passed
@Ingwannu
Ingwannu deleted the fix/760-management-origin-tls branch August 1, 2026 04:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants